-
Notifications
You must be signed in to change notification settings - Fork 8.3k
drivers: audio: sf32lb52x: audio codec driver #98701
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
download code from github very very slow. the old PR is: |
| - uart | ||
| - gpio | ||
| - watchdog | ||
| - audio-codec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - audio-codec | |
| - audio |
it should be used as a tag for some test/sample, otherwise has no utility.
https://docs.zephyrproject.org/latest/develop/test/twister.html#board-configuration
Speaking of which, this driver has to be tested (or at least built) in CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
gmarull
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure to not make unrelated changes into a big commit, split changes into chunks as needed. Also, make sure CI passes and do not open new PRs.
9625992 to
4f8250e
Compare
| @@ -0,0 +1,30 @@ | |||
| .. zephyr:code-sample:: codec | |||
| :name: codec | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| :name: codec | |
| :name: audio codec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
| sample: | ||
| description: onchip audio codec example | ||
| application | ||
| name: codec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| name: codec | |
| name: audio codec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
drivers/audio/sf32lb_codec.c
Outdated
|
|
||
| sys_set_bit(reg + CODEC_PLL_CFG2, AUDCODEC_PLL_CFG2_RSTB_Pos); | ||
|
|
||
| #if AVDD_V18_ENABLE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It this should be configured in dts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not now. I will remove the #if
drivers/audio/sf32lb_codec.c
Outdated
| key = irq_lock(); | ||
| sys_set_bit(cfg->reg + CODEC_ADC_CH0_CFG, AUDCODEC_ADC_CH0_CFG_DMA_EN_Pos); | ||
| irq_unlock(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why need to disable the interrupt while writting this register?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be multiple thread to access to this register? configuration might be interrupted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should consider if there is 'race condition' from interface level or application level, not in register access level
drivers/audio/sf32lb_codec.c
Outdated
| key = irq_lock(); | ||
| sys_set_bit(cfg->reg + CODEC_DAC_CH0_CFG, AUDCODEC_DAC_CH0_CFG_DMA_EN_Pos); | ||
| irq_unlock(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above
|
|
||
| sys_write32(reg_val, reg + CODEC_DAC_CH0_CFG_EXT); | ||
|
|
||
| reg_val = FIELD_PREP(AUDCODEC_DAC_CH0_DEBUG_BYPASS_Msk, 0) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It need to overwrite the value, so no read is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reg_val need initialized with 0 coz | operator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, reg_val is composed, no 0 init needed.
|
|
||
| static void config_rx_channel(uintptr_t reg, struct sf32lb_codec_adc_cfg *cfg) | ||
| { | ||
| uint32_t reg_val; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reg_val need init?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it will be overwritten before it is used.
|
|
||
| static void pll_turn_on(uintptr_t reg) | ||
| { | ||
| uint32_t reg_val; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it will be overwritten before it is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reg_val need initialized with 0 coz | operator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, reg_val is composed in next line. No need to init as 0.
| sys_set_bit(reg + CODEC_PLL_CFG3, AUDCODEC_PLL_CFG3_EN_SDM_Pos); | ||
| sys_set_bit(reg + CODEC_PLL_CFG4, AUDCODEC_PLL_CFG4_EN_CLK_DIG_Pos); | ||
|
|
||
| reg_val = FIELD_PREP(AUDCODEC_PLL_CFG1_R3_SEL_Msk, 3) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It need to overwrite the value, so no read is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if NOT all bits overwrite, some bits is still ramdom, take care
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the FIELD_PREP related code, and fixed those need to clear first.
9794b44 to
11858fe
Compare
drivers/dma/dma_sf32lb.c
Outdated
| LOG_ERR("start not possible with DMA enabled"); | ||
| return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return 0 when error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
drivers/dma/dma_sf32lb.c
Outdated
| uint32_t cm0arx; | ||
|
|
||
| ret = check_dma_config(channel, config_dma, config); | ||
| if (ret) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ret not boolean, use ret < 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
drivers/dma/dma_sf32lb.c
Outdated
| } | ||
|
|
||
| if (config_dma->dest_data_size != config_dma->source_data_size) { | ||
| LOG_ERR("dest_data_size != source_data_size not support"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Destination and source sizes not equal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
drivers/dma/dma_sf32lb.c
Outdated
|
|
||
| static int dma_sf32lb_config(const struct device *dev, uint32_t channel, | ||
| struct dma_config *config_dma) | ||
| static inline int check_dma_config(uint32_t channel, struct dma_config *config_dma, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why an extra inline? is it used anywhere else? if not please remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
| #define DMAC_CBSR2 offsetof(DMAC_TypeDef, CBSR2) | ||
| #define DMAC_CBSRX(n) (DMAC_CBSR1 + (DMAC_CBSR2 - DMAC_CBSR1) * (n)) | ||
| #define DMAC_CSELR1 offsetof(DMAC_TypeDef, CSELR1) | ||
| #define DMAC_CSELR2 offsetof(DMAC_TypeDef, CSELR2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve commit description for f942b8b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
| }; | ||
| }; | ||
|
|
||
| pa_power_default: pa_power_default { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
| .. zephyr:code-sample:: codec | ||
| :name: audio codec | ||
|
|
||
| mic to speaker loopback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename sample folder to a more descriptive name, e.g. mic_speaker_loopback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not just loopback, it has playback function as well.
| #if DT_NODE_HAS_STATUS_OKAY(DT_NODELABEL(audcodec)) | ||
| dev = DEVICE_DT_GET(DT_NODELABEL(audcodec)); | ||
| #else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this makes sample specific to SF32, please, rely on standard e.g. aliases to make sample portable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
audcodec is not SF32 specific, this is just a new audio device name
| if (!device_is_ready(dev)) { | ||
| printk("codec device is not ready\n"); | ||
| return -EBUSY; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
device readiness needs to be done at the very beginning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| static uint8_t __aligned(4) pcm_16k[] = { | ||
| 0x01, 0x00, 0xFE, 0x17, 0x55, 0x2C, 0xEB, 0x39, 0xB1, 0x3E, 0xEC, 0x39, 0x55, 0x2C, 0xFE, | ||
| 0x17, 0x01, 0x00, 0x02, 0xE8, 0xAC, 0xD3, 0x15, 0xC6, 0x4F, 0xC1, 0x15, 0xC6, 0xAC, 0xD3, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is data needed if sample is a mic loopback>?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sample has 2 demos, one is playback, the other is loopback
aec9f04 to
9efd2c3
Compare
5f203b9 to
5a0675b
Compare
| static void pa_power_disable(const struct gpio_dt_spec *spec) | ||
| { | ||
| /* configure PA power pin */ | ||
| (void)gpio_pin_configure_dt(spec, GPIO_OUTPUT_LOW); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| (void)gpio_pin_configure_dt(spec, GPIO_OUTPUT_LOW); | |
| gpio_pin_configure_dt(spec, GPIO_OUTPUT_INACTIVE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pin is not LED, it has to be low to disable, high to enable PA.
| static void pa_power_enable(const struct gpio_dt_spec *spec) | ||
| { | ||
| /* configure PA power pin */ | ||
| (void)gpio_pin_configure_dt(spec, GPIO_OUTPUT_HIGH); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| (void)gpio_pin_configure_dt(spec, GPIO_OUTPUT_HIGH); | |
| gpio_pin_configure_dt(spec, GPIO_OUTPUT_ACTIVE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pin is not LED, it has to be low to disable, high to enable PA.
Add callback of half complete in DMA processing, Fix DMA size bug. Signed-off-by: Gang He <[email protected]>
Add audio codec device tree information for sf32lb52x.dtsi Signed-off-by: Gang He <[email protected]>
Support audio device that has both input and output function. Signed-off-by: Gang He <[email protected]>
audio playback and capure, extend API in codec.h Signed-off-by: Gang He <[email protected]>
Add audio codec in board device tree, including PA control pin. Signed-off-by: Gang He <[email protected]>
Add audio loop back sample Signed-off-by: Gang He <[email protected]>
|



…m #97906
audio playback and capure, extend API in codec.h